Protect global ORTB consent fields at the level where the SDK writes them - #1009
Open
mdanylov-sigma wants to merge 1 commit into
Open
mdanylov-sigma wants to merge 1 commit into
mdanylov-sigma wants to merge 1 commit into
Conversation
OpenRtbMerger.removeSensitiveData applied several field lists at the wrong
nesting level, so a publisher-supplied global ORTB config could overwrite
consent signals the SDK computes:
- FIELDS_REGS was applied to "regs", but UserConsentParameterBuilder writes
GDPR and US Privacy into "regs.ext". Only "coppa" was actually protected;
{"regs":{"ext":{"gdpr":0}}} silently overrode the SDK's computed value.
- FIELDS_GEO was applied to a root-level "geo" object that does not exist in
OpenRTB, so the list never matched anything. "user.geo" was unprotected.
- "regs.gpp" and "regs.gpp_sid" are serialized by Regs but were not protected.
Each list is now applied where the SDK actually writes the field, mirroring
ArbitraryGlobalORTBHelper.ProtectedFields on iOS: separate regs/regs.ext and
user/user.ext lists. "regs.ext.tfua" stays open deliberately, since the global
ORTB config is the only way for publishers to send it (#997).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OpenRtbMerger.removeSensitiveDatastrips SDK-computed fields out of the publisher-supplied global ORTB config before merging, so that a publisher can enrich the bid request but cannot overwrite signals the SDK is responsible for. Three of its field lists were applied at the wrong nesting level, so they protected nothing.The result is a privacy-compliance bug: a global ORTB config could silently override the GDPR and US Privacy values the SDK computed from the consent storage, and could restore location precision the SDK had deliberately coarsened.
This PR re-applies each list at the level where the SDK actually writes the field, matching
ArbitraryGlobalORTBHelper.ProtectedFieldsin prebid-mobile-ios.Why this needs fixing
1.
regs.ext.gdprandregs.ext.us_privacywere never protectedFIELDS_REGS = {"gdpr", "us_privacy", "coppa"}was applied toopenRtbJson.optJSONObject("regs")— the top level ofregs. ButUserConsentParameterBuilderwrites two of those three intoregs.ext:So only
coppawas ever removed. A publisher config of:{ "regs": { "ext": { "gdpr": 0 } } }passed straight through the merger and overwrote the SDK's computed
gdprflag on the outgoing bid request. The same applies tous_privacy.This is the whole point of the sanitization step: these values come from the CMP via
UserConsentManager, and the SDK must be the single source of truth for them. A publisher able to flipgdprto0(whether by mistake in a hand-written config, or by copying an example blob) turns off downstream GDPR handling for every request in the app while the CMP says otherwise.2.
FIELDS_GEOwas dead code, anduser.geowas unprotectedFIELDS_GEOwas applied toopenRtbJson.optJSONObject("geo")— a root-levelgeoobject. There is no root-levelgeoin OpenRTB; geo lives atdevice.geoanduser.geo. The list never matched anything.device.geohappened to be covered anyway, becauseFIELDS_DEVICEalready contains"geo".user.geowas not covered by anything — and the SDK writes it:BasicParameterBuilder.appendUserTargetingParametersThat
applyLocationPrecisioncall is a deliberate privacy control — the publisher asked for coordinates to be truncated. Leavinguser.geoopen meant a global ORTB config could put full-precision coordinates back into the request, defeating the setting the app itself configured.3.
regs.gppandregs.gpp_sidwere missingBoth are serialized by
Regs.getJsonObjectand both are SDK-computed —UserConsentParameterBuilder.appendGppParameterfeeds them fromUserConsentManager.getRealGppString()/getRealGppSid(). They belong in the protected set for exactly the same reason asgdpr/us_privacy, and iOS already protects them.What changed
removeSensitiveDatanow pairs each object with its ownextlist, mirroring the iOSprops/extPropssplit:FIELDS_REGSregscoppa,gpp,gpp_sidFIELDS_REGS_EXTregs.extgdpr,us_privacyFIELDS_USERusergeoFIELDS_USER_EXTuser.extconsentFIELDS_DEVICEdeviceFIELDS_GEOis deleted. Geo is now protected wholesale by its parents (device.geoviaFIELDS_DEVICE,user.geoviaFIELDS_USER), which is what iOS does — no per-field geo list is needed.Alignment with iOS
ArbitraryGlobalORTBHelper.ProtectedFieldsin prebid-mobile-ios gets the nesting right today, and itsremoveProtectedFields(from:props:extProps:)takes both an object-level and anext-level list per object. After this PR the two SDKs agree on:regsProps→FIELDS_REGS(gpp_sid,gpp,coppa)regsExtProps→FIELDS_REGS_EXT(gdpr,us_privacy)userProps→FIELDS_USER(geo)userExtProps→FIELDS_USER_EXT(consent)Beyond correctness, parity matters here because publishers ship the same ORTB config JSON to both platforms. Today, a config containing
regs.ext.gdpris stripped on iOS and honoured on Android — the same app sends different consent signals depending on the platform, which is the worst possible failure mode for a compliance signal, since it is invisible unless you diff the two bid requests.Deliberate divergences (both intentional, both noted in code)
regs.ext.tfuastays unprotected. iOS doesn't protect it either, and per [Android SDK] Inquiries on Regional Child Privacy (tfua) & Impression-Level ORTB Config #997 the global ORTB config is currently the only way for publishers to sendtfuafor non-US child-protection regimes. There is now a comment onFIELDS_REGS_EXTso this isn't "tidied up" by a future change. A regression test covers it.device.extis still dropped wholesale on Android.FIELDS_DEVICEcontains"ext", so the entiredevice.extobject is removed; iOS only stripsattsandifvfrom it. Android's behaviour is the stricter of the two, and narrowing it would loosen protection, so it is left alone here. Worth a follow-up decision, but out of scope for a fix.Tests
Five new tests in
OpenRtbMergerTest:mergeSensitiveData_regsExtConsentFields_areNotOverridden— publisherregs.ext.gdpr/us_privacycannot overwrite SDK valuesmergeSensitiveData_regsExtConsentFields_areNotAddedToEmptyRequest— nor can they be injected when the SDK set nothingmergeSensitiveData_regsExtTfua_passesThrough—tfuasurvives while a siblinggdprin the same object is strippedmergeSensitiveData_regsCoppaGppAndGppSid_areNotOverriddenmergeSensitiveData_userGeo_isNotOverridden—user.geostripped while a siblinguser.keywordsstill merges normallyThe three
sensitive_data_*.jsonfixtures were regenerated. They previously encoded the wrong request shape (regs.gdprat the top level, a root-levelgeoobject), which is why the existing tests passed against the bug — they asserted the merger removed fields from a structure the SDK never produces.14/14 pass. The
rendering.networking.parameterspackage was also run as a regression check; the two failures inBasicParameterBuilderTest.whenSetPluginRendererList*are pre-existing onmasterand unrelated to this change.Doc comments
Regs.getJsonObjectcarried "When you add a new field to this list, don't forget to add it to the OpenRtbMerger". Sinceregsfields now split across two lists, it names both (FIELDS_REGSfor top-level,FIELDS_REGS_EXTforext). The identical comment ongeo/Geo.javahad become misleading — a newGeofield needs no merger change at all now — so it explains that the object is protected by its parent instead.Device.java's comment is still accurate and untouched.🤖 Generated with Claude Code